arch: rm support for init'ing attempt.prompt with str
#1361
Conversation
|
consider removing support for |
| message = entity.pop("content", {}) | ||
| if isinstance(message, str): | ||
| content = Message(text=message) | ||
| content = Message(message, str) |
There was a problem hiding this comment.
I don't think this revision is correct, str is a class and passing it as the second argument to this constructor would not be valid. In theory this branch of the conditional cannot be reached and probably should raise a ValueError if content in the serialized entity was of object type str. The existing code would provide a method to manually create a more loose approximation of a serialized Turn however do not I see any reason we should support it.
jmartin-tech
left a comment
jmartin-tech
left a comment
There was a problem hiding this comment.
Early review thoughts.
| elif isinstance(prompt, str): | ||
| msg = Message(text=prompt, lang=lang) | ||
| raise ValueError( | ||
| "attempt Prompt must be Message or Conversation, not string" | ||
| ) | ||
| # msg = Message(text=prompt, lang=lang) |
There was a problem hiding this comment.
Prefer to just remove this.
| if isinstance(value, str): | ||
| # note this does not contain a lang | ||
| self._prompt = Conversation([Turn("user", Message(text=value))]) | ||
| raise TypeError( | ||
| "Attempt.prompt must be Message or Conversation, not bare string" | ||
| ) | ||
| # self._prompt = Conversation([Turn("user", Message(text=value))]) |
There was a problem hiding this comment.
Again prefer to just remove this.
| d = _plugins.load_plugin("detectors.continuation.Continuation") | ||
|
|
||
| a = garak.attempt.Attempt(prompt="test prompts") | ||
| a = garak.attempt.Attempt(prompt=garak.attempt.Message(text="test prompts")) |
There was a problem hiding this comment.
Many of the edits in this file look ripe for a single CONSTANT.
| COMPLYING_OUTPUTS, REFUSAL_OUTPUTS = mitigation_outputs | ||
| d = garak.detectors.mitigation.MitigationBypass() | ||
| attempt = Attempt(prompt="testing prompt", lang=d.lang_spec.split(",")[0]) | ||
| d = garak._plugins.load_plugin("detectors.mitigation.MitigationBypass") |
There was a problem hiding this comment.
Was there a strong reason to change this to use load_plugin() or is this a consistency thing?
| d = MitigationBypass() | ||
| attempt = garak.attempt.Attempt(prompt="testing prompt", lang=d.lang_spec) | ||
| attempt = garak.attempt.Attempt( | ||
| prompt=garak.attempt.Message(text="testing prompt"), lang=d.lang_spec |
There was a problem hiding this comment.
While I don't think we actually have any detectors with multiple lang values in lang_spec this should probably be explicit.
| prompt=garak.attempt.Message(text="testing prompt"), lang=d.lang_spec | |
| prompt=garak.attempt.Message(text="testing prompt"), lang=d.lang_spec.split(",")[0] |
2 participants
fixes #1347
attempt.promptacceptsMessageorConversation- now we check for thishey seeing as local models are difficult to run, are perhaps the minority case, and are where most of our deps come from, what are thoughts on dropping these, and migrating to something strongly typed, like rust?
Verification